fix issue #252, support subgroup aesthetic in geom_polygon for holes#320
fix issue #252, support subgroup aesthetic in geom_polygon for holes#320Nishita-shah1 wants to merge 3 commits into
Conversation
|
Hi @tdhock , @Faye-yufan and @suhaani-agarwal . Could you please review this. |
| points.forEach(function(pt){ | ||
| var sg = pt.subgroup !== undefined ? pt.subgroup : "1"; | ||
| if(!rings_map.hasOwnProperty(sg)){ | ||
| rings_map[sg] = []; | ||
| ring_order.push(sg); | ||
| } | ||
| rings_map[sg].push([scales.x(pt.x), scales.y(pt.y)]); | ||
| }); | ||
| var coords = ring_order.map(function(sg){ | ||
| var ring = rings_map[sg]; | ||
| if(ring.length > 0){ | ||
| ring = ring.concat([ring[0]]); | ||
| } | ||
| return ring; | ||
| }); | ||
| var geojson = { | ||
| type: "Polygon", | ||
| coordinates: coords | ||
| }; | ||
| return geoPath(geojson); |
There was a problem hiding this comment.
I do not understand this code.
I was expecting some library function call here, instead of these forEach/map calls. is that possible?
or can you please add comments and/or link some documentation?
There was a problem hiding this comment.
forEach and map here are plain JavaScript built-in array methods, forEach loops over each point to group them by subgroup into rings_map, and map transforms those groups into closed GeoJSON rings.
Is there any other library function to use? Could you please tell me about it? I'll read about it and make the changes in the code.
Also, I looked into d3.group() (https://d3js.org/d3-array/group) as a possible replacement for the forEach grouping step. Would that be a good direction to explore?
There was a problem hiding this comment.
Also, I looked into d3.group() (https://d3js.org/d3-array/group) as a possible replacement for the forEach grouping step. Would that be a good direction to explore?
hi nishita! d3.group() is from D3v7+, but animint.js uses D3v3. take a look at how grouping is done elsewhere in this file (search for d3.nest). The same pattern could likely apply to your code.
you can try refactoring the forEach grouping block using the existing library pattern you find.
| var rings_map = {}; | ||
| var ring_order = []; | ||
| points.forEach(function(pt){ | ||
| var sg = pt.subgroup !== undefined ? pt.subgroup : "1"; |
There was a problem hiding this comment.
Hi @Nishita-shah1 , would g.data pass in null/na if subgroup is true? In what scenario that subgroup == undefined but g_info.data_has_subgroup == true?
Any test case for this scenario?
There was a problem hiding this comment.
Thank you mentors for the feedback .
I have updated the code and now it uses d3.nest.
https://docs.google.com/document/d/1QBB-OU3Gjli9TBWmX5k7ER3R3JqrCDDGI1usf22027I/edit?usp=sharing
in this doc i have written my logic and all the test cases.
There was a problem hiding this comment.
Hi @Nishita-shah1 , would g.data pass in null/na if subgroup is true? In what scenario that subgroup == undefined but g_info.data_has_subgroup == true? Any test case for this scenario?
Hi @Faye-yufan , i think it cannot happen because :
Step 1-
(R - geom-.r):data_has_subgroup is set ONLY when 'subgroup' %in% names(g.data) → column must physically exist in the data frame
Step 2- (JS — animint.js):
if(g_info.geom==='polygon' && g_info.data_has_subgroup), nly runs when R confirmed the column exists, pt.subgroup will always be a string ('1', '2', 'NA', etc.), never JS undefined
I have explained my logic and all the test cases in this doc https://docs.google.com/document/d/1QBB-OU3Gjli9TBWmX5k7ER3R3JqrCDDGI1usf22027I/edit?tab=t.0
Refactor polygon rendering to use d3.nest for subgroup aesthetics and GeoJSON with evenodd fill rule.
4 participants
Fixes #252
Problem
animint2 rendered isoband hole polygons as two separate filled shapes
instead of one polygon with a transparent hole. The
subgroupaesthetic(added to ggplot2 in 2019) was completely unknown to animint2's compiler
and renderer.
Real-world use case: Hi-C genomic cluster contour visualization where
holes in contour polygons are scientifically meaningful.
After (fix)
Solution
Three-layer fix across the full pipeline:
R/geom-polygon.r - added
subgroup = NULLtodefault_aessoGeomPolygon accepts the aesthetic.
R/geom-.r -- when
subgroupis present in polygon data:g$data_has_subgroup <- TRUE(serialized into plot.json)g$typesso JS type converter keeps it as stringinst/htmljs/animint.js - when
data_has_subgroupis true, usesd3.geo.path().projection(null)with GeoJSON Polygon format insteadof
d3.svg.line(). GeoJSON coordinates map directly to subgroupstructure:
[outerRing, holeRing1, ...]. SVGfill-rule: evenoddensures hole rings appear transparent.
Tests
17 tests, 0 failures covering:
no flag without subgroup
evenodd fill-rule applied
(hole_and_mid, only_hole, no_hole) as specified in the issue
@tdhock @suhaani-agarwal @Faye-yufan could you please review this, thank you! Fixes #252